Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

chore(core): log headers with _RIVET_LOG_HEADERS#1397

Closed
NathanFlurry wants to merge 1 commit into10-15-chore_example-next-js_improve_deploymentfrom
10-16-chore_core_log_headers_with__rivet_log_headers
Closed

chore(core): log headers with _RIVET_LOG_HEADERS#1397
NathanFlurry wants to merge 1 commit into10-15-chore_example-next-js_improve_deploymentfrom
10-16-chore_core_log_headers_with__rivet_log_headers

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 16, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Oct 16, 2025 10:00am

Copy link
Member Author

NathanFlurry commented Oct 16, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Oct 16, 2025

Claude finished @NathanFlurry's task —— View job


PR Code Review

Reviewing PR #1397: "chore(core): log headers with _RIVET_LOG_HEADERS"

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the modified file changes
  • Review for code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Check test coverage implications
  • Provide comprehensive feedback

Changes Summary:

  • File: packages/rivetkit/src/common/router.ts:40-42
  • Lines: +4/-1 (imported getEnvUniversal and added conditional header logging)
  • Purpose: Adds optional logging of all HTTP request headers when _RIVET_LOG_HEADERS environment variable is set

Code Review Feedback

Code Quality & Best Practices

  • Good: Follows repository logging guidelines (lowercase messages, structured logging)
  • Good: Uses existing getEnvUniversal utility function for cross-platform environment variable access
  • Good: Conditional feature activation via environment variable is appropriate for debugging functionality
  • Good: Uses spread operator for clean conditional object property inclusion

⚠️ Security Concerns

  • High Priority: Potential information disclosure risk - Logging all headers could expose sensitive information:
    • Authentication tokens (Authorization, Cookie headers)
    • API keys passed via custom headers
    • User session data
    • Internal routing information
  • Recommendation: Consider filtering out sensitive headers or add explicit warnings in documentation

🔍 Potential Issues

  • Memory/Performance: JSON.stringify(c.req.header()) on every request when enabled could impact performance in high-traffic scenarios
  • Log Volume: This will significantly increase log verbosity when enabled
  • Missing Documentation: No documentation about the _RIVET_LOG_HEADERS feature in code comments

💡 Suggestions for Improvement

  1. Security Enhancement - Consider filtering sensitive headers:
const SENSITIVE_HEADERS = ['authorization', 'cookie', 'x-api-key'];
const safeHeaders = Object.fromEntries(
  Object.entries(c.req.header()).filter(([key]) => 
    !SENSITIVE_HEADERS.includes(key.toLowerCase())
  )
);
  1. Add JSDoc Documentation:
// Add comment explaining the feature and security implications
...(getEnvUniversal("_RIVET_LOG_HEADERS")
  ? { allHeaders: JSON.stringify(c.req.header()) } // WARNING: May log sensitive data
  : {}),
  1. Performance Consideration: The current implementation is acceptable for debugging, but consider caching or rate limiting if this becomes a permanent feature.

📊 Test Coverage

  • Missing: No tests for the new conditional logging behavior
  • Recommendation: Add unit tests to verify:
    • Headers are logged when _RIVET_LOG_HEADERS is set
    • Headers are not logged when environment variable is unset
    • JSON serialization doesn't break with unusual header values

🎯 Overall Assessment

This is a straightforward debugging feature that's well-implemented from a code perspective. The main concern is the security implication of potentially logging sensitive headers. Consider the security enhancements suggested above before merging.

Approval Status: ⚠️ Conditional - Address security concerns before merging


@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 16, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1397

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1397

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1397

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1397

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1397

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1397

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1397

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1397

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1397

commit: f9826cd

@graphite-app
Copy link

graphite-app bot commented Oct 16, 2025

Merge activity

  • Oct 16, 7:43 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 16, 7:44 PM UTC: CI is running for this pull request on a draft pull request (#1403) due to your merge queue CI optimization settings.
  • Oct 16, 7:44 PM UTC: Merged by the Graphite merge queue via draft PR: #1403.

@graphite-app graphite-app bot closed this Oct 16, 2025
@graphite-app graphite-app bot deleted the 10-16-chore_core_log_headers_with__rivet_log_headers branch October 16, 2025 19:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant